-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a Windows platform native image (WCOW) #40
Conversation
0de5f62
to
f9d23a3
Compare
uses: docker/setup-qemu-action@v1 | ||
with: | ||
uses: docker/setup-qemu-action@v2 | ||
with: | ||
image: tonistiigi/binfmt:qemu-v6.0.0-12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one required? I'm not sure whether dependabot would notify about updates for that image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version pinning was required for linux/s390x builds, see #26.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the version pinning, because the linux/s390x builds would fail otherwise... although the version pinning had been introduces due to the same error which appears now 🤪
image: tonistiigi/binfmt:qemu-v6.0.0-12 | ||
- name: Set up Docker Buildx | ||
id: buildx | ||
uses: docker/setup-buildx-action@v1 | ||
uses: docker/setup-buildx-action@v2 | ||
with: | ||
version: v0.6.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one required? I'm not sure whether dependabot would notify about updates for that version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version pinning was required for linux/s390x builds, see #26.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the version pinning, because the linux/s390x builds would fail otherwise... although the version pinning had been introduces due to the same error which appears now 🤪
Woah, what are these issues on CI? |
|
AFAIK GHA uses Cygwin and bash for Windows when executing scripts, so that is something to keep in mind if we want to continue to make use of Make. However, the Makefile is not used anymore after all, or am I missing something? |
Btw., did you already rebase on #28? |
``` RUN go mod download go: github.com/Sirupsen/logrus@v1.0.6: Get "https://proxy.golang.org/github.com/%21sirupsen/logrus/@v/v1.0.6.mod": tls: invalid signature by the server certificate: ECDSA verification failure ```
@kiview I'm not sure if you (maintainers) would prefer using the Makefile for non-containerized builds. I have deleted it now as suggestion, but if you'd prefer to keep it, just leave a note :) This branch is now rebased on the current master branch. I also moved the Linux-Dockerfile to a subdirectory "linux". Are you going to squash the commits? If not, I can certainly cleanup a bit. |
We always merge with squash, so don't worry about the actual commit history 🙂 |
@mdelapenya yes, please, feel free to take over! :) |
Co-authored-by: Tobias Gesellchen <tobias@gesellix.de>
Co-authored-by: Tobias Gesellchen <tobias@gesellix.de>
Co-authored-by: Tobias Gesellchen <tobias@gesellix.de>
- name: Run Ryuk on Windows | ||
if: ${{ matrix.os == 'windows-2022' }} | ||
run: docker run -v //./pipe/docker_engine://./pipe/docker_engine testcontainers/ryuk:${RYUK_VERSION} | ||
continue-on-error: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line still required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we need to change it slightly, but we would like to keep something like this to test the built image. Due to the complexity and amount of combinations an error sneaked in. The linux/amd64
and windows/amd64
contained the ARM binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also think that the checks are required and make sense. I only asked for the "continue on error" line, which is at the very end of the workflow and doesn't have any effect (?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. continue-on-error
is not necessary. We will adjust it with the update that fixes the binaries. Unfortunately, for Windows more changes are necessary due to: actions/runner-images#6688.
@gesellix Hi 👋 We are thinking about revisiting this PR in order to get the windows container for Ryuk. We explored the options with goreleaser, although with very few success, as the ability to merge manifests coming from different builders is a Pro feature. Said that, I think it would be very beneficial if you and I can sync, probably in a zoom call or slack huddle, so we can make this PR into Ryuk. Wdyt? |
Hey @mdelapenya :) I'm going to contact you in Slack! |
We have a really productive online session today, where we were talking on how to unblock this issue after the recent changes in the build system of Ryuk and the failed intents of using goreleaser to build the app without using the Pro version of goreleaser. As a summary:
Therefore, I'm closing this PR waiting for the new one. Thanks all for participating in this healthy discussion 👏 |
I would like to mention that our cross-platform build essentially worked, but we encountered issues while publishing the multi-arch manifest to Docker Hub. The pipeline did not merge the manifest, instead it overwrote previously published manifests. |
I've restarted to work on the multi-arch manifest: gesellix#2 About the goreleaser images and using their manifests: I think that should work somehow, it all comes down to using explicit image references (can you share/link how goreleaser names all the published variants?), collecting their manifests, checking/annotating the I've tried to extract the "merger" step into a single job, maybe it can be refactored to a custom action with all the images/manifests as input and the desired multi-arch image name as output. |
Its been a while, but I think this are the latest changes we tested. The names are probably the value of |
I'll try (probably not before next week), thanks for the link! |
Hey guys. Are you still planning to merge either versions? Kind regards |
I've rebased the code at gesellix#2 to the current upstream (this repo). Let's see, if the workflows still work, which would also be a check, whether the concept stands the test of time :) |
Thanks for the quick reply @gesellix |
... something seems to be broken on the GitHub's side of Action Runners. I'll check later/tomorrow. Multi-Arch images should appear at https://github.com/gesellix/moby-ryuk/pkgs/container/moby-ryuk (with the older one at https://github.com/gesellix/moby-ryuk/pkgs/container/moby-ryuk/102573888?tag=wip). |
See https://github.com/gesellix/moby-ryuk/pkgs/container/moby-ryuk/182452091?tag=20240222.1 with a multi-arch image available for the proof of concept. Docker image name: |
A clean pull request is open for discussion at #110 |
PR was merged. When is it gonna be released? |
This adds a Windows image to the build, and to the published multi-arch image.
The Windows build ignores the Makefile, though. Please give some feedback if you'd prefer a more future-proof approach. I'd personally suggest to get rid of the Makefile and stick to the Dockerfiles. Another option would be to perform a cross-platform build using that Makefile and publishing the binaries for each platform as a package. Those binaries could then be used in the Dockerfiles.
I would also suggest to move the Linux Dockerfile to a subdirectorylinux
, so that it becomes more obvious that there are dedicated Dockerfiles for both Linux and Windows.Due to docker/buildx#176 and docker/build-push-action#18 we have to create the Windows image manually, and add it to the multi-arch manifest in another step. I've tested the workflow applied here for one of my own images, see https://github.com/gesellix/go-npipe/releases/tag/v2022-07-31T14-30-00.Edit: the current approach is based on https://lippertmarkus.com/2021/11/30/win-multiarch-img-lin/.Relates to #35
Relates to testcontainers/testcontainers-java#5621